fix(sql-parser): accept global aggregates without GROUP BY by milindsrivastava1997 · Pull Request #826 · ProjectASAP/ASAPQuery

milindsrivastava1997 · 2026-10-11T20:23:18Z

Summary

Before this change, SQL global aggregates without GROUP BY were rejected by SQLPatternParser, because its empty grouping-label set was treated as a parse failure. For example, these queries could not be planned:

SELECT SUM(value) FROM data
WHERE time BETWEEN DATEADD(s, -300, NOW()) AND NOW();

SELECT QUANTILE(0.95, value) FROM quantile_wide
WHERE time BETWEEN DATEADD(s, -1, NOW()) AND NOW();

After this change, an omitted GROUP BY parses as an empty grouping-label set. Single-layer global SUM, COUNT(column), MIN, MAX, QUANTILE, and COUNT(DISTINCT column) queries now plan and build an execution context, yielding a global result with no output grouping labels.

The parser also preserves empty grouping labels within nested SQL ASTs. Nested matching and execution remain intentionally unsupported and are deferred to #819.

Tests

cargo fmt --check
cargo test -p sql_utilities
cargo test -p query_engine_rust --test sql_query_support

Closes #817

milindsrivastava1997

Automated code review: 6 findings, posted inline. Findings 1, 2, 5 and 6 concern code outside this diff, so they are anchored on the Some(group_bys) change that makes those paths reachable.

milindsrivastava1997 · 2026-10-11T20:40:25Z

-                } else {
-                    Some(group_bys)
-                }
+                Some(group_bys)


[correctness] Global SUM/COUNT can match a label-keyed CMS via capability fallback (asap-query-engine/src/engines/simple_engine/sql.rs:258)

Global aggregates now reach find_compatible_aggregation with empty grouping_labels, and labels_compatible (capability_matching.rs:157) compares only grouping_labels, never aggregated_labels.

Scenario: the planner plans SELECT job, SUM(value) ... GROUP BY job, giving CountMinSketch(sum) with grouping=[] and aggregated=[job]. An unplanned SELECT SUM(value) FROM data ... has grouping=[], matches that config, and point-queries the empty key, returning ~0 instead of the total. Before this PR these queries were rejected at parse time.

Suggested fix: require aggregated_labels to be empty when the requirement has no grouping labels, or exclude configs with aggregated labels.

This was generated by AI during triage.

Fixed in c9bc666. Non-top-k global aggregates now require an exact query configuration instead of capability fallback, so a grouped CMS cannot be selected for an unlabeled scalar query. Added global_aggregate_does_not_fallback_to_grouped_config.

milindsrivastava1997 · 2026-10-11T20:40:25Z

-                } else {
-                    Some(group_bys)
-                }
+                Some(group_bys)


[correctness] Global COUNT also matches top-k CountMinSketchWithHeap configs (asap-query-engine/src/engines/simple_engine/sql.rs:258)

Statistic::Count lists CountMinSketchWithHeap as compatible, and cms_family_sub_type_compatible only checks count vs sum mode. Heap configs always have grouping=[].

Scenario: SELECT srcip, COUNT(value) AS c ... GROUP BY srcip ORDER BY c DESC LIMIT 5 (heap: grouping=[], aggregated=[srcip], count) plus an unplanned SELECT COUNT(value) FROM t .... The global COUNT is served by the srcip-keyed heap sketch and returns a bogus empty-key estimate instead of the event count.

This was generated by AI during triage.

Fixed by the same c9bc666 guard: a non-top-k global COUNT cannot capability-match a keyed heap configuration. Exact configurations remain eligible, and existing top-k capability-fallback tests still pass.

milindsrivastava1997 · 2026-10-11T20:40:25Z

-                } else {
-                    Some(group_bys)
-                }
+                Some(group_bys)


[correctness] Subquery path skips the unexpected-SELECT-fields check (sqlpattern_parser.rs:244)

Inner subqueries without GROUP BY now parse, but the subquery branch never runs the DISTINCT/HAVING/QUALIFY check that the flat path applies.

Scenario: SELECT SUM(t) FROM (SELECT SUM(value) AS t FROM x WHERE time BETWEEN ... HAVING SUM(value) > 5) used to be rejected by get_groupbys and now parses with the HAVING dropped. Only the matcher's NestedQueryUnsupported stops it today.

This was generated by AI during triage.

Deferred intentionally: this PR changes parsing only, while nested-query matching and execution remain unsupported. c9bc666 adds a matcher regression asserting NestedQueryUnsupported, so the parsed shape cannot enter execution. Tightening unsupported-clause validation inside nested parsing belongs with the nested-execution work.

milindsrivastava1997 · 2026-10-11T20:40:25Z

-                } else {
-                    Some(group_bys)
-                }
+                Some(group_bys)


[efficiency] Global SUM/COUNT plans a full CMS + DeltaSetAggregator for one scalar (asap-planner-rs/src/planner/sql.rs:124)

With zero aggregated labels, the planner allocates a multi-population CountMinSketch (width x depth) plus a key tracker for a population that is always one empty key. When spatial_output is empty, mapping to a single-population operator (Sum, or MinMax for exact) would be much cheaper.

This was generated by AI during triage.

Acknowledged and deferred. This PR is scoped to parser acceptance and safe execution of exact global configurations; choosing a cheaper single-population physical plan is a planner optimization and should be addressed separately.

milindsrivastava1997 · 2026-10-11T20:40:25Z

  - name: sum
    query: SELECT SUM(value) FROM data WHERE time BETWEEN DATEADD(s, -1, NOW()) AND NOW()
-    expected: unsupported
+    expected: supported


[test-coverage] Newly supported cases don't check result values (applies to all ~30 flipped cases; runner at asap-query-engine/tests/sql_query_support.rs:90)

The runner only checks that an execution context gets built. Nothing ingests data and asserts that global SUM/COUNT/MIN/MAX/QUANTILE/COUNT(DISTINCT) combines all series. If the empty-grouping path returned one row per series or an empty result, every case would still pass. The parser tests only assert labels.is_empty().

This was generated by AI during triage.

Fixed in c9bc666 with global_sum_returns_one_unlabeled_result. It inserts two global SUM windows, executes the full SQL pipeline, and asserts one empty-label row with value 8.0; this catches both duplicate-per-series and empty-result failures.

milindsrivastava1997 · 2026-10-11T20:40:25Z

+        )
+        .expect("nested global aggregates parse");
+
+        // Matching and execution may reject nested queries, but parsing retains both layers.


[test-coverage] No negative-path test for nested global aggregates

This test only shows that parsing keeps both layers. Nothing asserts that the matcher/planner then rejects the shape with NestedQueryUnsupported, so loosening the n > 1 check in query_info_to_pattern would let these flow into execution with time_info UNUSED and no test would fail. The "may reject" comment also leaves the expected outcome unstated.

This was generated by AI during triage.

Fixed in c9bc666. test_nested_global_aggregates_remain_unsupported_by_matcher now parses the nested global aggregate and asserts QueryError::NestedQueryUnsupported, making the accepted parser shape and rejected matcher shape explicit.

milindsrivastava1997 · 2026-10-12T13:08:03Z

This was generated by AI during triage.

PR-specific checks are green (format-and-lint, test, and both cross-language checks). The remaining Docker/image gates all fail while compiling the repository because Cargo references the missing asap-query-engine/src/bin/bench_ingest_sender.rs; each fails before exercising this PRs SQL changes. This leaves the PR merge-blocked by a separate build issue.

milindsrivastava1997 · 2026-10-12T13:50:52Z

Code review

simple_engine/mod.rs:1364: empty self-key set now queries fallback_key. The new Some(keys) if keys.is_empty() branch queries self-keyed accumulators with a key they were never indexed under. Before, this case emitted no rows. A grouped top-k (CountMinSketchWithHeap) with an empty heap now returns a made-up row labeled with the group key, carrying a CMS collision estimate. MultipleSum, MultipleMinMax and SetAggregator with an empty map and fallback_key=None now fail with "Key required" and log a warning on every step. PromQL shares this path.
simple_engine/sql.rs:509: the guard turns off fallback for all SQL without GROUP BY. That includes queries that a scalar config with grouping_labels=[] (Sum, KLL, HLL, MinMax) could serve correctly. With no query_configs match but a SumAccumulator config with grouping_labels=[], SELECT SUM(value) FROM t returns None.
simple_engine/sql.rs:506: the root cause is in the capability matcher. labels_compatible and find_compatible_aggregation accept a multi-population config for an unlabeled requirement, so PromQL sum(x) can still pick a MultipleSum config and return one row per inner key instead of a single scalar. Rejecting multi-population or self-keyed types in the matcher when req.grouping_labels is empty, or comparing aggregated_labels, would fix both paths and make the SQL-only guard unnecessary.
tests/sql_pattern_matching_tests.rs:142: global_aggregate_does_not_fallback_to_grouped_config passes without the guard. The helper builds the config grouped by [L1..L4], so labels_compatible already rejects a query with grouping []. Deleting sql.rs lines 506-511 leaves the test green. The test needs a config with grouping [] and a multi-population type.
simple_engine/mod.rs:3124: range output changed with no test. A None key now becomes an empty-label RangeVectorElement for every caller, including PromQL range queries. Range and sliding queries over ungrouped configs used to return an empty result and now return one unlabeled series. Only the instant path is tested.
tests/sql_pattern_matching_tests.rs:61: helper hardcodes [L1..L4]. It sets grouping labels from template_sql.contains("GROUP BY"), so a GROUP BY L1 template gets a config grouped by [L1..L4]. This is why test 4 passes for the wrong reason. Pass the grouping labels in explicitly instead.
simple_engine/mod.rs:1306: stale doc comment. The resolve_and_query_group doc still says self-keys always take priority and doesn't mention the new fallback query on an empty key set.
Dockerfile:34: unrelated change. The bench_ingest_sender stub fixes breakage from feat(bench): precompute-engine throughput vs CPU microbenchmark #825. Reverting this PR would break the Docker build again, so it belongs in its own commit or PR.

🤖 Generated with Claude Code

fix(sql-parser): accept global aggregates without group by

8a9c839

milindsrivastava1997 linked an issue Oct 11, 2026 that may be closed by this pull request

sql: aggregation without GROUP BY is rejected #817

Open

milindsrivastava1997 commented Oct 11, 2026

View reviewed changes

fix(query-engine): execute global SQL aggregates

c9bc666

fix(query-engine): stub benchmark binary in Docker build

2aaed9a

Conversation

milindsrivastava1997 commented Oct 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Tests

Uh oh!

milindsrivastava1997 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milindsrivastava1997 commented Oct 12, 2026

Uh oh!

milindsrivastava1997 commented Oct 12, 2026

Code review

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

milindsrivastava1997 commented Oct 11, 2026 •

edited

Loading